lightning-net crate
#1469
lightning-net crate
#1469
Conversation
|
Looks like a bunch of stuff broke after rebasing, on it. |
Squashed commits in original PR at #1: - Create empty lightning-net module - SyncSocketDescriptor skeleton implementation - Connection struct with read and write halves - Implement shutdown() for TcpReader and TcpWriter - Implement most of spawn_inbound_handler - Add crossbeam and some associated channels - Implement basic functionality of ConnectionWriter - Pass a Arc<PeerManager> into ConnectionReader - Get a SyncSocketDescriptor into the ConnectionWriter - Implement first half of pausing reads - Renome write tx/rx to write_data tx/rx - Rename ConnectionReader/Writer to Reader/Writer - Create the Reader/Writer cmd channels - Send ResumeRead command to the Reader - Remove write_cmd, rename reader_cmd -> resume_read - Reader can handle ResumeRead events - Implement disconnecting from peer - send_data() actually sends data now - Allow send_data() to pause reads - Get a Arc<PeerManager> into Writer - Refactor write_data tx/rx into writer_cmd tx/rx - Give Reader/Writer a cmd tx for the other - Implement all disconnecting except TcpDisconnectooor - Remove the Arc<Mutex<Connection>> - Refactor Connection into setup() - disconnect_socket() now calls into TcpDisconnectooor - Get a SyncSocketDescriptor into Writer - Call write_buffer_space_avail and socket_disconnected v2 - resume_read check should go before the early return - Handle read() ErrorKind variants - Implement handle_connection and initiate_outbound - Finish writing doc comments, clean up - Basic tests for lightning-net - These is modeled exactly after the tests in lightning-net-tokio - Update GitHub CI and dependabot for lightning-net - Reduce the dependencies used - Implementing PR review feedback from @phlip9 - Remove some comments about SGX - Hide ID_COUNTER in a function, return JoinHandles - Reader/Writer now send commands via the descriptor - Extract do_recv(), do_try_recv(), do_read() fns v2 - Split WriteData command into its own channel v4 - Remove `std::net` limitations from EDP doc comment - Implement ReaderState enum v2
Codecov Report
@@ Coverage Diff @@
## main #1469 +/- ##
==========================================
- Coverage 90.92% 90.75% -0.17%
==========================================
Files 75 78 +3
Lines 41842 42583 +741
Branches 41842 42583 +741
==========================================
+ Hits 38043 38645 +602
- Misses 3799 3938 +139
Continue to review full report at Codecov.
|
| /// | ||
| /// This buffer is necessary because calls to self.inner.write() may fail or | ||
| /// may write only part of the data. | ||
| buf: Option<Vec<u8>>, |
There was a problem hiding this comment.
rust-lightning should re-call with the remaining data if you only write part, I think you should be able to do the writer with no internal buffer at all. For that matter, I think you should be able to do the writer with no separate thread at all, either!
There was a problem hiding this comment.
I think you should be able to do the writer with no separate thread at all, either!
If I'm understanding you correctly, you mean to use TcpStream's set_nonblocking(true), and then do stream.write() directly within SyncSocketDescriptor::send_data()? That would work for the writer, but since the Reader and Writer share the same underlying TcpStream which is managed by the OS, this means that the reader would no longer be able to block on read(), and could only read in a busy loop. From the set_nonblocking() docs:
This will result in
read,write,recvandsendoperations becoming nonblocking, i.e., immediately returning from their calls. If the IO operation is successful,Okis returned and no further action is required. If the IO operation could not be completed and needs to be retried, an error with kindio::ErrorKind::WouldBlockis returned.On Unix platforms, calling this method corresponds to calling
fcntl FIONBIO. On Windows calling this method corresponds to callingioctlsocket FIONBIO
There was a problem hiding this comment.
rust-lightning should re-call with the remaining data if you only write part
This requires send_data() to know immediately whether a call to write() wrote all of the bytes, some of the bytes, etc. But since we cannot use non-blocking writes due to the constraints on the Reader above, there needs to be a dedicated Writer thread that can block on write(). Which AFAICT necessitates a layer of indirection (i.e. the write_data channel) between send_data() and the Writer, but which unfortunately makes partial write()s undetectable to callers of send_data().
There was a problem hiding this comment.
I think you should be able to do the writer with no internal buffer at all
I could imagine making the write_data channel a zero-capacity channel, so that when the Writer recv()s data, it immediately turns around and passes the data into write(), instead of storing it in an internal buffer. But since send_data() cannot detect partial writes (as explored above), the Writer must continue trying to send the remaining data until it succeeds (or shuts down), since the call to send_data() reported to Rust-Lightning that the entire write was a success. And in order to do that, the remaining data must be stored in memory somewhere, thus the internal buffer.
I suppose that making write_data a zero-capacity channel would prevent the channel itself from behaving as a second buffer (where the first buffer is the Writer's self.buf). This eliminates one of the buffers, but still doesn't eliminate the Writer's internal buffer.
I don't see a way around these constraints - any suggestions? The constraints are subtle but have wide-reaching implications regarding the design of this crate.
There was a problem hiding this comment.
Can you not set the write timeout to 1 microsecond? I'm not 100% sure how the platform-specific bits there work, so we'd need to test it on win/linux/bsd but I'd imagine it may work.
There was a problem hiding this comment.
I'm also really curious if there's not a way to somehow money-patch in a select call, or at a minimum ask Fortanix if they'd be interested in a minimal select-like API. They write in their ABI doc that one of their goals is to "support network services" but without select I would suggest it really doesn't. Before we stick with all the overhead here maybe it makes sense to ask (okay, beg) them to add select?
There was a problem hiding this comment.
If you're already thinking of adding custom usercalls, I'm not sure about the approach of running the TCP socket handling inside the enclave - I wrote this above, but it may have gotten missed in all the messages
we could also consider running the actual net logic outside of the enclave - it doesn't change the security model to make the syscalls outside of the enclave, we just pass the stream of read/write_buffer_space_avail events and write calls over some IPC. I'd bet we could do all that with less total LoC :/
There was a problem hiding this comment.
To rethink this somewhat further, instead of jumping through hoops here, we could also consider running the actual net logic outside of the enclave - it doesn't change the security model to make the syscalls outside of the enclave, we just pass the stream of read/write_buffer_space_avail events and write calls over some IPC. I'd bet we could do all that with less total LoC :/
That's essentially what we will be doing once the usercalls are implemented: making the syscalls outside of the enclave. Maybe take a quick look at the EDP architecture (it's short)
There was a problem hiding this comment.
We could then think about (a) a nice and performant ldk-net crate here (b) a little "ldk-net <-> PeerManager" shim trait/API here, and then (c) you could hook that shim to actually pass the events through to the enclave, but other users could hook it directly up to the PeerManager.
There was a problem hiding this comment.
That's essentially what we will be doing once the usercalls are implemented: making the syscalls outside of the enclave. Maybe take a quick look at the EDP architecture (it's short)
Right, it was not at all clear to me from that doc if the usercall API was designed to let you add your own calls or if it was designed only to let you use their small number of syscalls.
|
After the review by @TheBlueMatt, I think this module needs more comprehensive tests. A future contributor is likely not to be as familiar with all the constraints around threads blocking on |
|
Before you go toooo far down the rabbit hole of adding a ton of tests and documentation maybe lets figure out if we can remove the channels (and second thread) entirely first. I'm pretty confident we can which should massively reduce the total code surface here. |
|
Please excuse the delay; I got my computer repaired during the last week while I was with family. I still don't think the writer thread can be eliminated since there would be no way to call FYI I will be on a trip and have limited bandwidth to implement things from the 21st through the 29th, so no rush. |
Right, I'm still somewhat curious as to the exact SGX programming environment here - are you able to define your own "usercall"s, and if so, can we define a |
The custom usercalls are fairly limited. The main interface is the Essentially, I'd have to implement an RPC service from scratch. The enclave would call I think this is totally unnecessary though. On my lowest tier SGX Azure instance, the default maximum number of threads supported by the operating system is 31368. With Making the existing implementation work has already stalled our project by a few weeks. The easiest, lowest resistance path forward I see is to simply move ahead with the current implementation that uses two threads per connection, and implement a usercall to handle |
Ah, that's quite dissapointing. I guess it makes sense, but frustrating.
Right, my questions aren't strictly about thread count - each thread carries (by default) 8MB of stack dedicated to it, context switch latency when we go to do writes, etc. My poor little RPi with 35 channels would eat 560MB (1/4-1/3 of its available memory) just on threads dedicated to handling network tasks (assuming those threads use stack, in theory it could be left unallocated, though some per thread would always be allocated, and that's not counting OS/kernel overhead of threads).
Sorry to hear that, I think that's a pretty clear indication we need to provide a better higher-level network interface here. And of course there's no reason you can't use this crate for your own networking logic! It just may not make sense to take it upstream as something we encourage people to use (at least outside of the rather narrow use-case of SGX). |
|
https://doc.rust-lang.org/std/thread/struct.Builder.html#method.stack_size Thoughts on setting the thread stack size to eg 4KB? The biggest local variable is the 8kb buffer for the Reader, which can be heap allocated by using a Vec instead. |
Stack size isn't just the maximum of any single function, its the total stack allocations of every function in an entire call tree. For example, if I set the stack limit to 4KB ( I'd be willing to bet the maximum stack depth for calling LDK functions is at least 256K-512K, at least once you count OS thread setup (likely more in your case for SGX?) and syscalls. In general, though, I don't think there's a reason to reduce the stack size - the stack size is effectively a "maximum", which if we overrun it will cause (in the best case) a crash. Linux, however, can avoid actually allocating the stack space, or at least swap it out, until its used. Of course once its used you don't get the memory back. More generally, this also doesn't consider the OS overhead of threads, context switches, etc, which, sure, aren't much on modern machines, but its also not free. For running in SGX on a beefy machine its probably totally fine, but it may not be the direction we want to go for a general |
Ah, I forgot the calls into PeerManager - good point.
Noted. I did some digging on a My point is - if this crate is to be upstreamed, I think the primary selling point should be that it is maximally portable with minimal reliance on any platform-specific APIs, rather than that it can match the performance of As for me and Philip's SGX project, we could certainly remove the overhead of dedicated threads by implementing a usercall extension that uses futures / polling / similar outside the enclave, but then the implementation would be specific to the SGX platform and it wouldn't make any sense to upstream it. The EDP people did mention the possibility of supporting |
Sometimes I really really hate Rust. We have no-std and alloc separated out for a reason, std requires filesystem and TCP already,
More generally, I'd argue the SGX platform you're using already doesn't "support std" given it mis-implements a number of std features in ways that would no doubt break many applications, certainly any doing nontrivial TCP read/writes (like this crate!).
Ah, I think you may have missed my suggestion above - we can add a shim interface between the ldk-net crate and All that said, given you indicated you're behind where you want to be, I suggest we leave this for now. We can revisit an architecture that's easier to hook in the future when we have more time in the future. |
|
All understood, and good points. Thanks for being welcoming to new contributors and taking the time to iterate with me on this @TheBlueMatt; it was a pleasure, and I learned a lot :) Will go ahead and close this PR. |
lightning-net
Motivation
@philip9 and I are working on an initiative to compile and run LDK within an SGX enclave using the Fortanix Enclave Development Platform (EDP). Among the many additional constraints imposed when trying to run Rust code inside SGX is that we cannot use Tokio, specifically none of the
netfeatures that are relied upon in the current Tokio-based implementation of the rust-lightning network stacklightning-net-tokiobecause those features rely onmiowhich in turn relies onepoll-like syscalls which are not available in thex86_64-fortanix-unknown-sgx(EDP) compilation target.Description
This PR implements a new crate called
lightning-netwhich essentially reimplements the functionality oflightning-net-tokiobut without using Tokio, futures, async Rust, or anything else that will not compile to thex86_64-fortanix-unknown-sgxtarget.As the code currently stands, the ldk-sample node could easily swap out all usages of
lightning-net-tokiowith their equivalent functions inlightning-netand function perfectly fine. I've tested this already in a private fork of ldk-sample running on testnet - let me know if you'd like me to make a public version so that you can play around with it as well.Previous work
Prior to this pull request, @phlip9 and I iterated on this module on our own fork at lexe-tech/rust-lightning#1. We now feel it is ready for the next stage of review prior to being upstreamed.
Follow-on work
Currently,
lightning-netdoes not yet implement any SGX-specific functionality. In particular, we need to implement an SGX-compatible socket that has a subset of the existingTcpStreamfeatures. Follow-on work will implement this concreteSgxTcpStreamusing either custom Fortanix Usercalls (or possibly via patches to Ruststd), and modify thehandle_connection()andinitiate_outbound()functions to take in an impl of e.g. a newSockettrait. Since we are not yet settled on the final / best approach, and since this pull request is fairly beefy already at +1129 lines, we figure it would be best to upstream this code now and add in the generics / SGX functionality later.Original commits prior to squash
Squashed commits in original PR at lexe-tech/rust-lightning#1, reviewed by @phlip9:
std::netlimitations from EDP doc comment